Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v9.x-backport] Backport #17198, #17738, #17841, #17736, #17881, #17879 and #18139 #19006

Closed
wants to merge 7 commits into from

Conversation

apapirovski
Copy link
Member

As per title, backports a whole bunch of inter-dependent PRs.

Note that #17198 used to be marked as semver-major but several TSC members have approved it being downgraded as it's non-breaking and is a dependency for several subsequent changes.

There is at least one semver-minor PR in here so this can't land until 9.7.0 at the earliest.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

addaleax and others added 7 commits February 26, 2018 10:25
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

PR-URL: nodejs#17198
Fixes: nodejs#6802
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Do not share unnecessary information about nextTick state
between JS & C++, instead only track whether a nextTick
is scheduled or not.

Turn nextTickQueue into an Object instead of a class
since multiple instances are never created.

Other assorted refinements and refactoring.

PR-URL: nodejs#17738
Reviewed-By: Anna Henningsen <[email protected]>
When a process encounters a _fatalException that is caught, it should
schedule execution of nextTicks but not in an arbitrary place of the
next Immediates queue. Instead, add a no-op function to the queue
that will ensure processImmediate runs, which will then ensure
that nextTicks are processed at the end.

PR-URL: nodejs#17841
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

PR-URL: nodejs#17736
Fixes: nodejs#17681
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#17881
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

PR-URL: nodejs#17879
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

PR-URL: nodejs#18139
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Feb 26, 2018
@apapirovski
Copy link
Member Author

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 26, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

addaleax added a commit that referenced this pull request Feb 26, 2018
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Backport-PR-URL: #19006
PR-URL: #17198
Fixes: #6802
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Do not share unnecessary information about nextTick state
between JS & C++, instead only track whether a nextTick
is scheduled or not.

Turn nextTickQueue into an Object instead of a class
since multiple instances are never created.

Other assorted refinements and refactoring.

Backport-PR-URL: #19006
PR-URL: #17738
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
When a process encounters a _fatalException that is caught, it should
schedule execution of nextTicks but not in an arbitrary place of the
next Immediates queue. Instead, add a no-op function to the queue
that will ensure processImmediate runs, which will then ensure
that nextTicks are processed at the end.

Backport-PR-URL: #19006
PR-URL: #17841
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Backport-PR-URL: #19006
PR-URL: #17881
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

Backport-PR-URL: #19006
PR-URL: #17879
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

Backport-PR-URL: #19006
PR-URL: #18139
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member

addaleax commented Feb 26, 2018

Thanks, landed on v9.x-staging!

landed in 743cf33...f2dd17b

@addaleax addaleax closed this Feb 26, 2018
addaleax added a commit that referenced this pull request Feb 26, 2018
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Backport-PR-URL: #19006
PR-URL: #17198
Fixes: #6802
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Do not share unnecessary information about nextTick state
between JS & C++, instead only track whether a nextTick
is scheduled or not.

Turn nextTickQueue into an Object instead of a class
since multiple instances are never created.

Other assorted refinements and refactoring.

Backport-PR-URL: #19006
PR-URL: #17738
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
When a process encounters a _fatalException that is caught, it should
schedule execution of nextTicks but not in an arbitrary place of the
next Immediates queue. Instead, add a no-op function to the queue
that will ensure processImmediate runs, which will then ensure
that nextTicks are processed at the end.

Backport-PR-URL: #19006
PR-URL: #17841
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Backport-PR-URL: #19006
PR-URL: #17881
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

Backport-PR-URL: #19006
PR-URL: #17879
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

Backport-PR-URL: #19006
PR-URL: #18139
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Backport-PR-URL: #19006
PR-URL: #17198
Fixes: #6802
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Do not share unnecessary information about nextTick state
between JS & C++, instead only track whether a nextTick
is scheduled or not.

Turn nextTickQueue into an Object instead of a class
since multiple instances are never created.

Other assorted refinements and refactoring.

Backport-PR-URL: #19006
PR-URL: #17738
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
When a process encounters a _fatalException that is caught, it should
schedule execution of nextTicks but not in an arbitrary place of the
next Immediates queue. Instead, add a no-op function to the queue
that will ensure processImmediate runs, which will then ensure
that nextTicks are processed at the end.

Backport-PR-URL: #19006
PR-URL: #17841
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Backport-PR-URL: #19006
PR-URL: #17881
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
If an error is encountered during the processing of Immediates, schedule
the remaining queue to finish after all error handling code runs (if the
process is still alive to do so). The new changes make the Immediates
error handling behaviour entirely deterministic and predictable, as the
full queue will be flushed on each Immediates cycle, regardless of
whether an error is encountered or not.

Currently this processing is scheduled for nextTick which can yield
unpredictable results as the nextTick might happen as early as close
callbacks phase or as late as after the next event loop turns Immediates
all fully processed. The latter can result in two full cycles of
Immediates processing during one even loop turn.

The current implementation also doesn't differentiate between Immediates
scheduled for the current queue run or the next one, so Immediates that
were scheduled for the next turn of the event loop, will process
alongside the ones that were scheduled for the current turn.

Backport-PR-URL: #19006
PR-URL: #17879
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Refactor Immediates handling to allow for them to be unrefed, similar
to setTimeout, but without extra handles.

Document the new `immediate.ref()` and `immediate.unref()` methods.

Add SetImmediateUnref on the C++ side.

Backport-PR-URL: #19006
PR-URL: #18139
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@apapirovski apapirovski deleted the backport-17738 branch April 22, 2018 10:17
@MylesBorins
Copy link
Contributor

@apapirovski should we be backporting all these to v8.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants